React Fabric: Support passing nativeViewTag to getInspectorDataForViewAtPoint callback, for React DevTools compat #21080

JoshuaGross · 2021-03-24T22:54:28Z

React Fabric: Support passing nativeViewTag to getInspectorDataForViewAtPoint callback, for React DevTools compat

Summary

React DevTools needs a React Tag to highlight a component. Fabric tries not to expose this, but we can expose it "temporarily" ;) for React DevTools compat.

React DevTools will need to be refactored in the future to take a HostComponent instance.

Test Plan

yarn flow fabric && yarn flow native && yarn lint && yarn test

Test inspector in Fabric, on iOS and Android (videos coming in a bit)

…wAtPoint callback, for React DevTools compat

sizebot · 2021-03-24T22:58:57Z

Comparing: 148f8e4...9543825

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	122.62 kB	122.62 kB	=	39.45 kB	39.45 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	129.02 kB	129.02 kB	=	41.43 kB	41.43 kB
facebook-www/ReactDOM-prod.classic.js	=	407.89 kB	407.89 kB	=	75.53 kB	75.53 kB
facebook-www/ReactDOM-prod.modern.js	=	396.14 kB	396.14 kB	=	73.62 kB	73.62 kB
facebook-www/ReactDOMForked-prod.classic.js	=	407.89 kB	407.89 kB	=	75.53 kB	75.53 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 9543825

rickhanlonii

cc @bvaughn

rickhanlonii · 2021-03-24T23:07:01Z

packages/react-native-renderer/src/ReactNativeFiberInspector.js

          nativeFabricUIManager.measure(
            internalInstanceHandle.stateNode.node,
            (x, y, width, height, pageX, pageY) => {
+              const inspectorData = getInspectorDataForInstance(
+                getClosestInstanceFromNode(closestInstance),


What's this needed for?

the change is just a refactor so this codepath looks like the non-Fabric path

JoshuaGross · 2021-03-25T00:46:57Z

I'm fairly sure this is a reasonable, and maybe correct change to make, and it does get us further along in the Inspector... but still doesn't make inspect work 100% yet with the devtools. Not quite sure what's missing.

JoshuaGross · 2021-03-25T00:47:40Z

Well, in terms of correctness, what we want to do longterm is have DevTools reference host components and not ReactTag if possible. But as a hack, this should work. But there's still some gap.

bvaughn · 2021-03-25T17:06:27Z

packages/react-native-renderer/src/ReactNativeFiberInspector.js

          nativeFabricUIManager.measure(
            internalInstanceHandle.stateNode.node,
            (x, y, width, height, pageX, pageY) => {
+              const inspectorData = getInspectorDataForInstance(
+                closestInstance,
+              );
              callback({


Regarding why this might not work, is it possible that the code path a few lines up is actually the one being called? (I can breakpoint this later and actually see, just a drive by comment)

react/packages/react-native-renderer/src/ReactNativeFiberInspector.js

Lines 183 to 189 in 4a99c5c

if (internalInstanceHandle == null) {

callback({

pointerY: locationY,

frame: {left: 0, top: 0, width: 0, height: 0},

...getInspectorDataForInstance(closestInstance),

});

}

I verified that the expected code-path is being hit, so that shouldn't be the root-cause. At that point it would require knowledge of/debugging the DevTools themselves (or the websocket bridget?) so I didn't get much further. But we are getting to this point, and we do have the correct nativeTag, so I'm not sure what's up

bvaughn · 2021-03-25T19:01:09Z

For anyone (with access to fbsource) reading this, here is how I test changes like this.

Changes to React DevTools

In the React repo

In packages/react-devtools-core run yarn build
Run jf upload ./dist/backend.js and copy the download string.

In on-demand mobile (or a local fbsource checkout)

Go to the React DevTools NPM checkout: cd ~/xplat/js/public/node_modules/react-devtools-core/dist
Remove the NPM installed backend: rm ./backend.js
Pull down the locally built one: jf download <<hash>> --file "backend.js"

Changes to React Native renderer

In on-demand mobile (or a local fbsource checkout)

Go to xplat: cd ~/xplat
Run the React Native sync script with a reference to the PR number in question: js1 upgrade react --pull=21080

bvaughn · 2021-03-25T21:44:16Z

Quick update that I tried to verify whether this worked or not and was unable to. Catalyst crashed on me and then I couldn't get DevTools frontend and backend to connect. (On-demand port tunneling does not seem to be working correctly for me.)

This PR looks like a good fix to me. Maybe we should just land it as-is (after fixing the lint issue). I don't think it would make things worse at least :D

JoshuaGross · 2021-03-25T23:23:57Z

Landed. The next RN sync will pull this in and we can keep iterating.

bvaughn · 2021-03-26T00:17:33Z

Thanks Josh!

Summary: This sync includes the following changes: - **[c9aab1c9d](facebook/react@c9aab1c9d )**: react-refresh@0.10.0 //<Dan Abramov>// - **[516b76b9a](facebook/react@516b76b9a )**: [Fast Refresh] Support callthrough HOCs ([#21104](facebook/react#21104)) //<Dan Abramov>// - **[0853aab74](facebook/react@0853aab74 )**: Log all errors to console.error by default ([#21130](facebook/react#21130)) //<Sebastian Markbåge>// - **[d1294c9d4](facebook/react@d1294c9d4 )**: Add global onError handler ([#21129](facebook/react#21129)) //<Sebastian Markbåge>// - **[64983aab5](facebook/react@64983aab5 )**: Remove redundant setUpdatePriority call ([#21127](facebook/react#21127)) //<Andrew Clark>// - **[634cc52e6](facebook/react@634cc52e6 )**: Delete dead variable: currentEventWipLanes ([#21123](facebook/react#21123)) //<Andrew Clark>// - **[1102224bb](facebook/react@1102224bb )**: Fix: flushSync changes priority inside effect ([#21122](facebook/react#21122)) //<Andrew Clark>// - **[dbe98a5aa](facebook/react@dbe98a5aa )**: Move sync task queue to its own module ([#21109](facebook/react#21109)) //<Andrew Clark>// - **[3ba5c8737](facebook/react@3ba5c8737 )**: Remove Scheduler indirection ([#21107](facebook/react#21107)) //<Andrew Clark>// - **[46b68eaf6](facebook/react@46b68eaf6 )**: Delete LanePriority type ([#21090](facebook/react#21090)) //<Andrew Clark>// - **[dcd13045e](facebook/react@dcd13045e )**: Use Lane to track root callback priority ([#21089](facebook/react#21089)) //<Andrew Clark>// - **[5f21a9fca](facebook/react@5f21a9fca )**: Clean up host pointers in level 2 of clean-up flag ([#21112](facebook/react#21112)) //<Andrew Clark>// - **[32d6f39ed](facebook/react@32d6f39ed )**: [Fizz] Support special HTML/SVG/MathML tags to suspend ([#21113](facebook/react#21113)) //<Sebastian Markbåge>// - **[a77dd13ed](facebook/react@a77dd13ed )**: Delete enableDiscreteEventFlushingChange ([#21110](facebook/react#21110)) //<Andrew Clark>// - **[048ee4c0c](facebook/react@048ee4c0c )**: Use `act` in fuzz tester to flush expired work ([#21108](facebook/react#21108)) //<Andrew Clark>// - **[556644e23](facebook/react@556644e23 )**: Fix plurals ([#21106](facebook/react#21106)) //<Sebastian Markbåge>// - **[8b741437b](facebook/react@8b741437b )**: Rename SuspendedWork to Task ([#21105](facebook/react#21105)) //<Sebastian Markbåge>// - **[38a1aedb4](facebook/react@38a1aedb4 )**: [Fizz] Add FormatContext and Refactor Work ([#21103](facebook/react#21103)) //<Sebastian Markbåge>// - **[1b7e471b9](facebook/react@1b7e471b9 )**: React Native New Architecture: Support passing nativeViewTag to getInspectorDataForViewAtPoint callback, for React DevTools compat ([#21080](facebook/react#21080)) //<Joshua Gross>// - **[4a99c5c3a](facebook/react@4a99c5c3a )**: Use highest priority lane to detect interruptions ([#21088](facebook/react#21088)) //<Andrew Clark>// - **[77be52729](facebook/react@77be52729 )**: Remove LanePriority from computeExpirationTime ([#21087](facebook/react#21087)) //<Andrew Clark>// - **[3221e8fba](facebook/react@3221e8fba )**: Remove LanePriority from getBumpedLaneForHydration ([#21086](facebook/react#21086)) //<Andrew Clark>// - **[05ec0d764](facebook/react@05ec0d764 )**: Entangled expired lanes with SyncLane ([#21083](facebook/react#21083)) //<Andrew Clark>// - **[03ede83d2](facebook/react@03ede83d2 )**: Use EventPriority to track update priority ([#21082](facebook/react#21082)) //<Andrew Clark>// - **[a63f0953b](facebook/react@a63f0953b )**: Delete SyncBatchedLane ([#21061](facebook/react#21061)) //<Ricky>// - **[fa868d6be](facebook/react@fa868d6be )**: Make opaque EventPriority type a Lane internally ([#21065](facebook/react#21065)) //<Andrew Clark>// - **[eb58c3909](facebook/react@eb58c3909 )**: react-hooks/exhaustive-deps: Handle optional chained methods as dependency ([#20204](facebook/react#20204)) ([#20247](facebook/react#20247)) //<Ari Perkkiö>// - **[7b84dbd16](facebook/react@7b84dbd16 )**: Fail build on deep requires in npm packages ([#21063](facebook/react#21063)) //<Dan Abramov>// - **[2c9d8efc8](facebook/react@2c9d8efc8 )**: Add react-reconciler/constants entry point ([#21062](facebook/react#21062)) //<Dan Abramov>// - **[d0eaf7829](facebook/react@d0eaf7829 )**: Move priorities to separate import to break cycle ([#21060](facebook/react#21060)) //<Andrew Clark>// - **[435cff986](facebook/react@435cff986 )**: [Fizz] Expose callbacks in options for when various stages of the content is done ([#21056](facebook/react#21056)) //<Sebastian Markbåge>// - **[25bfa287f](facebook/react@25bfa287f )**: [Experiment] Add feature flag for more aggressive memory clean-up of deleted fiber trees ([#21039](facebook/react#21039)) //<Benoit Girard>// - **[8fe7810e7](facebook/react@8fe7810e7 )**: Remove already completed comment ([#21054](facebook/react#21054)) //<Sebastian Markbåge>// - **[6c3202b1e](facebook/react@6c3202b1e )**: [Fizz] Use identifierPrefix to avoid conflicts within the same response ([#21037](facebook/react#21037)) //<Sebastian Markbåge>// - **[dcdf8de7e](facebook/react@dcdf8de7e )**: Remove discrete lanes and priorities ([#21040](facebook/react#21040)) //<Andrew Clark>// - **[ca99ae97b](facebook/react@ca99ae97b )**: Replace some flushExpired callsites ([#20975](facebook/react#20975)) //<Ricky>// - **[1fafac002](facebook/react@1fafac002 )**: Use SyncLane for discrete event hydration ([#21038](facebook/react#21038)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 6d3ecb7...c9aab1c jest_e2e[run_all_tests] Reviewed By: JoshuaGross Differential Revision: D27436763 fbshipit-source-id: da79a41e26bffdcdacd293178062edf098e9b58a

…wAtPoint callback, for React DevTools compat (facebook#21080) React Fabric: Support passing nativeViewTag to getInspectorDataForViewAtPoint callback, for React DevTools compat

React Fabric: Support passing nativeViewTag to getInspectorDataForVie…

e1840e1

…wAtPoint callback, for React DevTools compat

facebook-github-bot added the CLA Signed label Mar 24, 2021

JoshuaGross requested a review from yungsters March 24, 2021 22:54

rickhanlonii approved these changes Mar 24, 2021

View reviewed changes

update

d6dbb6c

bvaughn reviewed Mar 25, 2021

View reviewed changes

prettier

9543825

JoshuaGross merged commit 1b7e471 into facebook:master Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React Fabric: Support passing nativeViewTag to getInspectorDataForViewAtPoint callback, for React DevTools compat #21080

React Fabric: Support passing nativeViewTag to getInspectorDataForViewAtPoint callback, for React DevTools compat #21080

JoshuaGross commented Mar 24, 2021

sizebot commented Mar 24, 2021 •

edited

Loading

rickhanlonii left a comment

rickhanlonii Mar 24, 2021

JoshuaGross Mar 24, 2021

JoshuaGross commented Mar 25, 2021

JoshuaGross commented Mar 25, 2021

bvaughn Mar 25, 2021 •

edited

Loading

JoshuaGross Mar 25, 2021

bvaughn commented Mar 25, 2021

bvaughn commented Mar 25, 2021

JoshuaGross commented Mar 25, 2021

bvaughn commented Mar 26, 2021

	if (internalInstanceHandle == null) {
	callback({
	pointerY: locationY,
	frame: {left: 0, top: 0, width: 0, height: 0},
	...getInspectorDataForInstance(closestInstance),
	});
	}

React Fabric: Support passing nativeViewTag to getInspectorDataForViewAtPoint callback, for React DevTools compat #21080

React Fabric: Support passing nativeViewTag to getInspectorDataForViewAtPoint callback, for React DevTools compat #21080

Conversation

JoshuaGross commented Mar 24, 2021

Summary

Test Plan

sizebot commented Mar 24, 2021 • edited Loading

Critical size changes

Significant size changes

rickhanlonii left a comment

Choose a reason for hiding this comment

rickhanlonii Mar 24, 2021

Choose a reason for hiding this comment

JoshuaGross Mar 24, 2021

Choose a reason for hiding this comment

JoshuaGross commented Mar 25, 2021

JoshuaGross commented Mar 25, 2021

bvaughn Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

JoshuaGross Mar 25, 2021

Choose a reason for hiding this comment

bvaughn commented Mar 25, 2021

Changes to React DevTools

In the React repo

In on-demand mobile (or a local fbsource checkout)

Changes to React Native renderer

In on-demand mobile (or a local fbsource checkout)

bvaughn commented Mar 25, 2021

JoshuaGross commented Mar 25, 2021

bvaughn commented Mar 26, 2021

sizebot commented Mar 24, 2021 •

edited

Loading

bvaughn Mar 25, 2021 •

edited

Loading